Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#772) refactored / simplified parameters to KnResultCollector #773

Merged
merged 4 commits into from
Apr 8, 2020

Conversation

maximilien
Copy link
Contributor

Description

Simplify and consolidated parameters to KnResultCollector

Changes

  • pass testing.T to KnResultCollector
  • pass test.KnTest to KnResultCollector
  • change functions calls to only using new KnResultCollector parameters

Reference

Fixes #772

/lint

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 1, 2020
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximilien: 2 warnings.

In response to this:

Description

Simplify and consolidated parameters to KnResultCollector

Changes

  • pass testing.T to KnResultCollector
  • pass test.KnTest to KnResultCollector
  • change functions calls to only using new KnResultCollector parameters

Reference

Fixes #772

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

test/e2e/traffic_split_test.go Outdated Show resolved Hide resolved
test/e2e/service_options_test.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 1, 2020
@maximilien
Copy link
Contributor Author

/ok-to-test
/assign @navidshaikh

@knative-prow-robot knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 1, 2020
@maximilien
Copy link
Contributor Author

Please ignore the golint Godoc issues as they are solved in #771 which I can rebase into this when its merged.

cc @navidshaikh

@maximilien
Copy link
Contributor Author

/test pull-knative-client-integration-tests-latest-release

test/e2e/version_test.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2020
@maximilien
Copy link
Contributor Author

Ok to merge @navidshaikh

@@ -28,10 +28,13 @@ import (
func TestVersion(t *testing.T) {
t.Parallel()

r := test.NewKnRunResultCollector(t)
it, err := test.NewKnTest()
Copy link
Collaborator

@navidshaikh navidshaikh Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewKnTest() will create a namespace for this test.

  1. We don't need namespace creation for version test.
  2. We dont have tearDown in this test, which leaving behind the created namespace.
    Ran this locally from PR:
➜  client git:(c8c54c52) go test ./test/e2e/ -count=1 -test.v --tags e2e -run TestVersion                                                                        
=== RUN   TestVersion
=== PAUSE TestVersion
=== CONT  TestVersion
--- PASS: TestVersion (5.54s)
PASS
ok  	knative.dev/client/test/e2e	5.544s

➜  client git:(c8c54c52) kubectl get ns|grep kne2etests
kne2etests0                                             Active   56s

If kn doesn't require a namespace to execute, lets not create one? Call to NewKnTest() will create one.

I'd still suggest the earlier review comment fix for version test, IMO no need to define a new method for only version test case.

func TestVersion(t *testing.T) {
  t.Parallel()
  r := test.NewKnRunResultCollector(t, nil)
  defer r.DumpIfFailed()
  out := test.RunKn("", []string{"version"})
  r.AssertNoError(out)
  assert.Check(t, util.ContainsAll(out.Stdout, "Version"))
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go without this, we'll have a namespace left behind from e2e test run which will break running e2e locally against same cluster in next run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a way to run w/o namespace is important since remember that this code is now library that can be used outside. I added code to tear down the namespace when the test is done:

+	defer func() {
+		assert.NilError(t, it.Teardown())
+	}()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note also that the new code:

// RunNoNamespace the 'kn' CLI with args but no namespace
func (k Kn) RunNoNamespace(args ...string) KnRunResult {
	return RunKn("", args)
}

Is pretty much just a wrapper to make clear we are running kn on default namespace which is a useful thing in general. Keeps the VersionTest consistent with the other tests. We still have various e2e tests that need to be converged to this model so I think it's best to keep the simple VersionTest in line with the others since people will use that as an exemplar.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as test.RunKn is an exported method, this could actually be an example for how to run kn without creating a namespace in tests. Say a plugin does not require a namespace to perform its task, its e2e can refer version tests.

@maximilien
Copy link
Contributor Author

/retest

Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Lets get this in. I've added a comment on the version tests discussion about how plugins not requiring namespace to perform their tasks can use test.RunKn.

@@ -28,10 +28,13 @@ import (
func TestVersion(t *testing.T) {
t.Parallel()

r := test.NewKnRunResultCollector(t)
it, err := test.NewKnTest()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as test.RunKn is an exported method, this could actually be an example for how to run kn without creating a namespace in tests. Say a plugin does not require a namespace to perform its task, its e2e can refer version tests.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [maximilien,navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 395fc6c into knative:master Apr 8, 2020
@navidshaikh navidshaikh added backport/candidate Consider this PR to be backported to the release branch and removed backport/candidate Consider this PR to be backported to the release branch labels Apr 15, 2020
@navidshaikh navidshaikh added this to the v0.14.0 milestone Apr 20, 2020
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
…d build 0.6 release (knative#773)

* add CI flow for eventing, eventing-sources and build

* update config.yaml
dsimansk added a commit to dsimansk/client that referenced this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: simplify parameters in e2e tests
4 participants